Stricter and more complete typechecking #1148
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
meta-cla
bot
added
the
CLA Signed
Contributor
|
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D87432029. (Because this pull request was imported automatically, there will not be any future comments.) |
cormacrelf
force-pushed
the
typecheck
branch
2 times, most recently
from
1c08c5b to
ef204f2
Compare
cormacrelf
force-pushed
the
typecheck
branch
from
ef204f2 to
ab12dcf
Compare
These are all failing.
This adds BindingType. This distinguishes annotated types from solved ones, and means we no longer try to solve types that the user annotated.
We need to do this without stopping the walk of the AST so that LSP can continue typechecking if it wants.
Do not allow `x=5; x: str = ""`, or `x: int = ...; x: str = ...`, or `def func(): pass; def func(): pass`. This is invalid in python, so should not be valid starlark. Updates a golden test, which made this error. Also removes a `def assert_eq` in another test as the function already exists.
This is invalid python, and starlark is meant to be a subset. There was a test that this at least parsed, but such annotations were completely ignored anyway.
For use in BindingsCollect.
Previously, we gathered and solved bindings for every top-level def, and not for a module's root bindings. This was a deliberate limitation for typechecker performance reasons, introduced in D48752028/D48752027 back in 2023. This completes the work described in those diffs, which said ideally: 1. typecheck top-level assignments 2. then evaluate defs This does one better and typechecks even nested defs individually. This achieves the goal of reducing the iterations needed in solve_bindings by passing the smallest possible quantity of bindings to solve_bindings at any one time.
Add a type check for any assignment to a binding that was annotated
cormacrelf
force-pushed
the
typecheck
branch
from
ab12dcf to
df6a9ef
Compare
Contributor
|
FWIW this looks great at a high level, but we don't have a lot of starlark people to review this PR, and it's a very big change. Could you split it up so we can work incrementally? For example, the toplevel bindings checks sounds like it could be separate PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a number of issues in the typechecker. I think it's fair to say before this PR we were only really checking function call sites and return types. The improvements here make type annotations on regular bindings significantly more useful. In general, we behave a lot more like a Python typechecker, as I looked at what basedpyright does in these situations and made starlark do that.
x: str = ""checked the immediately assigned expression, but no other assignments tox. In factxwas fed to the binding solver anyway, which meant a binding could hold whatever you wanted it to. Now we obey the annotation for all assignments, including locals from function parameters. That's almost all of the churn in the prelude, reassigning to function parameters with a different type.x: str = ...; x: int = ...was accepted and was equivalent toint | strbecause of point (1). Now this is disallowed, you can only annotate a binding once.x[0]: int = 5was allowed and ignored. No longer.defs, so this manages to feed even fewer bindings tosolve_bindingsat once than before.x: str = ...; x, y = (1, 2)was caught as a type error.I fixed the prelude's type errors, using some separate LSP improvements beyond the ones I've submitted already.
As for reviewers, maybe @ndmitchell? A few old diff messages refer to you as knowing the most about this.
A couple of TODOs for me: